Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(web): replace NetworkClient with queries #1519

Merged
merged 50 commits into from
Aug 16, 2024

Conversation

teclator
Copy link
Contributor

@teclator teclator commented Aug 1, 2024

Adapt the network management code to use queries instead of NetworkCient. Related to #1439.

Additionally, starts migrating the code to TypeScript and fixes bugs found by the type system.


Reviewers, please note that this ended up being a rather long PR, reason why is better to not only review the code but also to do manual testing if possible. Take your time, but any issue out of the scope of state management / queries migration should be reported as a new issue to be addressed in a new PBI.


Notes for creating follow-up work,

  • Fix WifiConnectionForm to avoid "error": "Network system error: Network state error: Connection '<ssid>' already exists" when connecting to hidden network that previously failed. In short, we should either, to drop the configuration created for a failed { hidden: true } connection or to implement a mechanism to know that such a configuration already exists and performs an update instead of add.
  • Review when it's possible to define a gateway and fix the UI information accordingly
  • Check if we can use the type Address from ipaddr package instead of our own IPAddress type (most probably not)
  • Add support for IPv6 since as @jreidinger said its usage is becoming more common. Look for FIXME: IPv6 is not supported yet. comments

@teclator teclator changed the title Getting rid of network client [WIP] Getting rid of network client Aug 1, 2024
@coveralls
Copy link

coveralls commented Aug 1, 2024

Coverage Status

coverage: 71.302%. remained the same
when pulling d588c38 on network_query_continue
into c992adc on master.

dgdavid and others added 26 commits August 5, 2024 09:04
Needed for building HTML ids without spaces from networks SSID since
spaces into an `aria-labelledby` attr are used as ids separator.
Keeping only the ssid, if any, from previous data stored in the query
since it's the unique data needed to allow the needsAuth update
performed on a failed device event.
It's no longer needed after previous changes improving the filtering and
mapping.
And include few code style changes done by prettier.
* Support for needsAuth flag.
* Support for adding or updating a connection, depending on the network
  settings.
* Fixed unit tests.
Migrating the file from jsx to tsx because the ConnectionsTable
component was already migrated to TypeScript.

const HIDDEN_NETWORK = Object.freeze({ hidden: true });
type HiddenNetwork = { hidden: boolean };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this type quite smell for me as I expect that hidden network contain more entries...why WifiNetwork cannot be used? or maybe instead of const variable method that do that simple check for hidden value on common WifiNetwork type can be used?

Copy link
Contributor

@dgdavid dgdavid Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me revisit this and see how we can improve it. I think yesterday we added the hidden attribute to WifiNetwork type, which was not the case before and probably the reason why this weird, ad-hoc type is here.

Good catch!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see now. WifiNetwork is an AccessPoint plus WifiNetworkStatus plus other optional values like hidden. So, HIDDEN_NETWORK cannot be a WifiNetwork sinde it does belong to none access point. A bit lost here, so I prefere to wait until @teclator can have a look.

type SelectedWifi = {
ssid?: string;
hidden?: boolean;
needsAuth?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so selected wifi can be empty? it looks kind of strange. I expect that at least ssid is mandatory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not that strange. You can connect to a hidden Wi-Fi network.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is strange for me as hidden network has SSID. The difference is that AP does not broadcast that SSID, so it is not visible when you scan list of available network. But to be able to connect to it, you need to know SSID. Just try it with your NM applet, when you want to connect to hidden network you have to provide its name

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know how hidden network works :) But the problem is that this is a "UI query" for keeping which network is/was selected by the user in order to show the proper information about it: the connection form or its configuration/state and actions.

But the user could select a "hidden network", for which the UI does not know the SSID in advance. That's why the ssid here is optional. Maybe a matter of naming or a matter or typing improvement (I'm just landing in the typed languages world :P)

How would you improve it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least document that info in the type as it is confusing for reader. So mention that for hidden network ssid is not known in advance. I am not sure if you can have something in ts like that hidden is true or ssid is specified.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if you can have something in ts like that hidden is true or ssid is specified.

Interesting point. I'm going to see if that it's possible. Thanks a ton!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh! Actually is a bit more tricky than I though because I forgot to mention a small detail in previous explanations: we're updating the information in the query in a moment in which we don't know the ssid but only that the authentication failed. So, we assumed that authentication failed for the selected connection. See https://github.com/openSUSE/agama/pull/1519/files#diff-f1b1879261cce23d74bb1716c65fbbcb487ee658acf074133fc6d6def7288736R223-R263

So, to make it work without TypeScript complaints the changes would be something like

diff --git a/web/src/queries/network.ts b/web/src/queries/network.ts
index 00754d6d..d8fecc7a 100644
--- a/web/src/queries/network.ts
+++ b/web/src/queries/network.ts
@@ -193,16 +193,25 @@ const useSelectedWifi = (): { ssid?: string; needsAuth?: boolean; hidden?: boole
 };
 
 const useSelectedWifiChange = () => {
-  type SelectedWifi = {
-    ssid?: string;
-    hidden?: boolean;
-    needsAuth?: boolean;
+  type VisibleWifi = {
+    ssid: string;
+    hidden?: false | never;
   };
 
+  type HiddenWifi = {
+    ssid?: never;
+    hidden: true;
+  };
+
+  type KnownWifi = VisibleWifi & { needsAuth?: boolean };
+
+  type SelectedWifi = KnownWifi | HiddenWifi;
+
   const queryClient = useQueryClient();
 
   const mutation = useMutation({
-    mutationFn: async (data: SelectedWifi): Promise<SelectedWifi> => Promise.resolve(data),
+    mutationFn: async (data: Partial<SelectedWifi>): Promise<Partial<SelectedWifi>> =>
+      Promise.resolve(data),
     onSuccess: (data: SelectedWifi) => {
       queryClient.setQueryData(["wifi", "selected"], (prev: SelectedWifi) => ({
         ssid: prev.ssid,

On one hand, from the typing POV would be more clear that a hidden wifi is unknown until the user enters the ssid and became it in a KnownWifi. On the other hand... although safer and robust code looks a bit complicated. What do you think?

@jreidinger
Copy link
Contributor

finished with code review. Manual test is currently ENOTIME.

@dgdavid
Copy link
Contributor

dgdavid commented Aug 13, 2024

@teclator, please be aware that I've merged master on this one to keep it up-to-date. See 02220c0

@teclator teclator merged commit d7eec2a into master Aug 16, 2024
3 checks passed
@teclator teclator deleted the network_query_continue branch August 16, 2024 07:13
@imobachgs imobachgs mentioned this pull request Sep 20, 2024
imobachgs added a commit that referenced this pull request Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants